Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

korean families r01: added #1459

Merged
merged 16 commits into from
Mar 13, 2018
Merged

korean families r01: added #1459

merged 16 commits into from
Mar 13, 2018

Conversation

m4rc1e
Copy link
Collaborator

@m4rc1e m4rc1e commented Feb 23, 2018

Korean Font binaries have been mastered by Aaron Bell, https://www.sajatypeworks.com

Browser diff images
https://drive.google.com/file/d/1qKvlXHytLheXr2fmaVoCWAm5Grwi0gvR/view?usp=sharing

FB reports
logs.zip

These fonts don't clear FB well. A lot of the errors are related to our test suite. Tomorrow, I will highlight these and file relevant errors to FB.

I've ran every font through diffbrowsers and they all work on Win7 IE9 to OSX Safari. Some of the diffs in IE9 contain black areas. This is caused by Browserstack, not the fonts.

I still have further fonts to pr so keep this open. I'll revise the commit message and this pr text.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Feb 23, 2018

Some of the diffs in IE9 contain black areas. This is caused by Browserstack, not the fonts.

It happened for NanumMyeongjo-Bold.

desktop_windows_7_ie_9 0_

When I rerun it through diffbrowsers, its fine.

desktop_windows_7_ie_9 0_

I'll send a mail to the Browserstack team

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Feb 24, 2018

Ok, I've added all the other families apart from:

  • Gaeugu
  • Sunf
  • KoreaDokdo
  • KoreanHBSJ
  • KoreanStirr

Gaeugu and Sunf don't have a Regular weight. They do have Mediums weights. @davelab6 shall I rename the Medium weights to Regular?

I think we should rename the fonts which have "Korea" in the name.

Still keep this open. I need to do further tests. I've updated the Nanum fonts so I'd like to show theirs been an improvement.

@davelab6
Copy link
Member

shall I rename the Medium weights to Regular?

Yes :) cc @aaronbell

@aaronbell
Copy link
Collaborator

I don't think there'd be issues with switching Medium to Regular (where no Regular exists).

@@ -1,16 +1,16 @@
name: "Nanum Brush Script"
name: "Nanum Brush"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do that! :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be good to maintain consistency between "Nanum Brush Script" and "Nanum Pen".

So either "Nanum Brush Script" and "Nanum Pen Script" or "Nanum Brush" and "Nanum Pen".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today on https://fonts.google.com/?subset=korean they both have "script" appended

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Well never mind me then :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nanum Pen Script on GF is a bit strange.

Font name in ttf is Nanum Pen
screen shot 2018-02-25 at 10 49 23

In the metadata file we have Nanum Pen Script.

The font names in this pr match the font names in the fonts hosted on GF. Can we not just amend the METADATA.pb files?

@davelab6
Copy link
Member

I'm going to hold off merging this until the family names are confirmed :)

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Feb 28, 2018

Just pushed an update which updates the font names.

Please don't merge this yet. I'm investigating an installation warning on OS X.

screen shot 2018-02-28 at 15 07 34

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Feb 28, 2018

The Font Book error message is caused when font nametable records are empty.

e.g

''
u'Yeon Sung'
u'Regular'
u'1.00;SAND;YeonSung-Regular'
u'Yeon Sung Regular'
u'Version 1.00'
u'YeonSung-Regular'
u'This Font Software is licensed under the SIL Open Font License, Version 1.1. This license is available with a FAQ at: http://scripts.sil.org/OFL'
''
u'Yeon Sung'
u'Regular'
u'1.00;SAND;YeonSung-Regular'
u'Yeon Sung Regular'
u'Version 1.00'
u'YeonSung-Regular'
u'This Font Software is licensed under the SIL Open Font License, Version 1.1. This license is available with a FAQ at: http://scripts.sil.org/OFL'
u'http://scripts.sil.org/OFL'

Many of the fonts in this pr have blank copyright strings. @davelab6 shall I remove the record or should I insert a placeholder like "Copyright the {{ Family }} Project Authors". Either way, it will solve this issue

@davelab6
Copy link
Member

Project authors please

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 1, 2018

@davelab6 I'm done but have a few questions.

in ofl/hanna I added the suffix .korean to the existing font which is served on early access

screen shot 2018-03-01 at 11 41 58

Is this correct?

I also think I should squash these commits and write a good pr message outlining what we've done.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 1, 2018

Cute Font also fails ots so I have removed it. Round tripping ttx did not solve it. Shall i readd it anyway?

@davelab6
Copy link
Member

davelab6 commented Mar 1, 2018

The fonts already on Early Access should be dealt with in another PR.

Nothing that fails OTS should be added.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 2, 2018

The fonts already on Early Access should be dealt with in another PR.

Nothing that fails OTS should be added.

Ok, here's my plan.

  1. Do not PR Hanna since it conflicts with the early access family. I checked Early Access and Hanna is the only conflict we have.
  2. Keep Cute Font out because it fails OTS
  3. Squash these prs and write meaningful commit msg

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@davelab6
Copy link
Member

davelab6 commented Mar 9, 2018

@m4rc1e is this ready to merge?

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 9, 2018

Yes

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Mar 12, 2018

@davelab6 All families are now added.

CuteFont works just fine with the removed GPOS table.

Win 7 IE9
desktop_windows_7_ie_9 0_

@davelab6 davelab6 merged commit 16680f8 into google:master Mar 13, 2018
@davelab6
Copy link
Member

  • Copyright notice in Black Han Sans was bad
  • Font file name mismatches with what we have in METADATA.pb (Single Day, Hi Melody, Poor Story, Gamja Flower)
  • fsSelection failures
    • NanumGothic-ExtraBold.ttf [fsSelection bold should be 1, is 0]
    • NanumMyeongjo-ExtraBold.ttf [fsSelection bold should be 1, is 0]
    • NanumMyeongjo-Bold.ttf [fsSelection bold should be 0, is 1]
    • NanumGothic-Bold.ttf [fsSelection bold should be 0, is 1]

davelab6 added a commit that referenced this pull request Mar 13, 2018
@m4rc1e m4rc1e mentioned this pull request Apr 4, 2018
@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Apr 6, 2018

I've started collecting my adhoc font engineering scripts

The scripts for this project can be found here

@davelab6
Copy link
Member

davelab6 commented Apr 7, 2018 via email

@m4rc1e m4rc1e mentioned this pull request Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants